You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Provides a very rudimentary chapter view for audiobooks now that jellyfin/jellyfin#16518 has been merged. I had grand plans for progress tracking but the itemDetails page isn't on React yet, so the required modifications were a pain.
I also tried a list.js replacement but that entire component probably needs to be rearchitected. Didn't want to tackle that at the same time.
Just so you're aware, I also made a chapters PR (amongst a bunch of things). To your specific change, I did a similar fix but ended up using the scenes section, but only on multi file audiobooks. To me this looks fine, if this gets merged in before mine I'll probably update mine to make sure multi-file uses your new section. Personally I have no preference either. Nice work
Played around with this and I believe there's a slight problem. When you're on the item page and edit the metadata it reloads and gives an error in the JS console and the section gets removed.
HTML wise all children of #childrenCollapsible .itemsContainer get removed, but both #childrenCollapsible & .itemsContainer do not get the hide class added, so it's not a can of being hidden, it's the html gets wiped out and not re-added (I assume because of the JS error).
Ok I figured out the issue: renderChapters reuses same .itemsContainer on every metadata refetch, without unmounting prior root first. Two React roots fight over same node: removeChild crash. Also no else branch to add hide class back when chapters missing.
3 solutions:
Create a new dedicated section that isn't reusing another one (and follows the same structure as option 2 below)
Follow roughly the suggestion below (notice I'm setting hide when audiobook but no chapters & I'm setting the unmount instead of on element I'm setting it on the new renderTarget AND more importantly I'm setting innerHTML = '' which is helping to clear it out:
function renderChapters(page, instance, item) {
if (item.Type !== BaseItemKind.AudioBook) return;
const container = page.querySelector('#childrenCollapsible');
const element = container.querySelector('.itemsContainer');
if (!item.Chapters?.length) {
container.classList.add('hide');
return;
}
element.innerHTML = '';
const renderTarget = document.createElement('div');
element.appendChild(renderTarget);
instance._unmount.push(renderComponent(ItemDetailsChapterList, { item }, renderTarget));
container.classList.remove('hide');
element.classList.add('vertical-list');
}
(My preferred): The reason I used the renderScenes function in my PR is that it already calls item.Chapters. The only reason this doesn't currently work for single file audiobooks is because the embedded chapters don't have images. So if you simply change:
if (chapters.length && !chapters[0].ImageTag) {
to
if (chapters.length && !chapters[0].ImageTag && item.Type !== BaseItemKind.AudioBook) {
it automagically works. The only caveat is the look isn't the prettiest. To fix that go to chaptercardbuilder.js:
I know, "basic chapter list for audiobook items". That's why there's the other 2 options. That being said, this is IMHO still a small change and makes it look better. Options 1 & 2 look like this:
@jordanmichaelrushing thanks for the analysis - pushed a small fix. @sevenrats your pull request is objectively more useful since it properly tracks playback state, but I'm going to try and get this into 12.0 before a released is tagged.
@sevenrats I forgot to mention the reason: your PR has ballooned to 3000 LOC so there's no chance for a 12.0 merge. We can shift gears once the new release comes out.
@sevenrats I forgot to mention the reason: your PR has ballooned to 3000 LOC so there's no chance for a 12.0 merge. We can shift gears once the new release comes out.
yeah that makes sense. if this direction changes please let me know asap because my PR is actually 1 of a series of PR's so if we are going to change direction I want as much notice as possible to try and preserve improvements that are downstream of my chapters PR.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
Provides a very rudimentary chapter view for audiobooks now that jellyfin/jellyfin#16518 has been merged. I had grand plans for progress tracking but the itemDetails page isn't on React yet, so the required modifications were a pain.
I also tried a
list.jsreplacement but that entire component probably needs to be rearchitected. Didn't want to tackle that at the same time.Issues
None
Code Assistance
None